Skip to content

Fix that ViFindBrace doesn't search for brace when current char is not a brace#4862

Open
sharpchen wants to merge 2 commits intoPowerShell:masterfrom
sharpchen:vifindbrace
Open

Fix that ViFindBrace doesn't search for brace when current char is not a brace#4862
sharpchen wants to merge 2 commits intoPowerShell:masterfrom
sharpchen:vifindbrace

Conversation

@sharpchen
Copy link
Copy Markdown

@sharpchen sharpchen commented Jul 22, 2025 •
edited
Loading

PR Summary

Vi searches right pair of first left brace, or first right brace when current char is not a brace. This pr added this functionality.

After the update, ViGotoBrace should work like this(of course this will also affect other methods such as ViDeleteBrace):
foo

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Make sure you've added one or more new tests
  • Make sure you've tested these changes in terminals that PowerShell is commonly used in (i.e. conhost.exe, Windows Terminal, Visual Studio Code Integrated Terminal, etc.)
  • User-facing changes
    • Not Applicable
    • OR
    • Documentation needed at PowerShell-Docs
      • Doc Issue filed:
Microsoft Reviewers: Open in CodeFlow

@Aston32
Copy link
Copy Markdown

Aston32 commented Mar 17, 2026

S

@andyleejordan
Copy link
Copy Markdown
Member

andyleejordan commented Mar 18, 2026

You are right that's how it works. I'm hoping we can do two things: verify the order of precedence in vanilla Vi (I was playing around trying to determine it and couldn't make heads or tails of it), and add some unit tests. Thanks! Love the fix though.

@sharpchen
Copy link
Copy Markdown
Author

sharpchen commented Mar 18, 2026

verify the order of precedence in vanilla Vi

Not sure what it means? If you mean the standard behavior, :h % documented the expected behavior in vim:

Find the next item in this line after or under the cursor and jump to its match.

And :h cpo-% described how the Vi behavior differs the way of Vim enhanced:

Parens inside single and double quotes are also counted, causing a string that contains a paren to disturb the matching. For example, in a line like "if (strcmp("foo(", s))" the first paren does not match the last one.

It indeed requires some tests. I did find at least one inconsistent behavior.

@sharpchen
Copy link
Copy Markdown
Author

sharpchen commented Mar 18, 2026

Hi @andyleejordan. I've rewrote the implementation, I think it's now good to go.
But I fail to run the test using ./build.ps1 -Test, it seems test/MovementTest.VI.cs is ignored? Tests pass even on false conditions.

andyleejordan
andyleejordan reviewed Mar 18, 2026
View reviewed changes
PSReadLine/Movement.vi.cs
// find next of any kind of paren
for (; nextParen < _buffer.Length; nextParen++)
for (int idx = 0; idx < parenthese.Length; idx++)
if (parenthese[idx] == _buffer[nextParen]) goto Outer;
Copy link
Copy Markdown
Member

@andyleejordan andyleejordan Mar 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rare great use of goto

@andyleejordan
Copy link
Copy Markdown
Member

andyleejordan commented Mar 18, 2026

I'm not sure but if this build is like others (looking at PSScriptAnalyzer right now), -Test won't rebuild so you have to run ./build.ps1 after any change and then follow it with ./build.ps1 -Test.

AppVeyor is at least running tests, looks like Test.en_US_Windows.ViGotoBrace and Test.ScreenReader.ViGotoBrace need to be checked and updated.

@sharpchen sharpchen force-pushed the vifindbrace branch 3 times, most recently from 611243c to fee3566 Compare March 18, 2026 23:26
@sharpchen
Copy link
Copy Markdown
Author

sharpchen commented Mar 19, 2026

Looks like tests are only available on Windows.

Shouldn't call ReadKey when there are no more keys

I honestly have no idea why test's failing. There's is a input but claims no key.

@andyleejordan
Copy link
Copy Markdown
Member

andyleejordan commented Mar 19, 2026

Looks like tests are only available on Windows.

I forgot about that but yes this is unfortunately true.

@andyleejordan
Copy link
Copy Markdown
Member

andyleejordan commented Mar 31, 2026

@sharpchen I will get this built on Windows and see if I can't fix the test so we can get this in!

@andyleejordan
Copy link
Copy Markdown
Member

andyleejordan commented Apr 1, 2026

Claude Opus 4.6 was actualy very adapt at fixing this error (it helps that the test was failing for the same reason as other similar tests, which then already had a fix it applied).

@andyleejordan
Fix ViGotoBrace tests
85ec2c2 
These tests need to be cleared with `ddi` and then
asserted to be empty. It's the same fix for the
other tests whose input contains unmatched braces.
Copilot's plausible explanation is that the
unmatched braces cause the input to be flagged by
PowerShell's parser as incomplete, so
`AcceptLineImpl` instead waits for more input,
causing the exception.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andyleejordan andyleejordan andyleejordan left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sharpchen @Aston32 @andyleejordan